perf: pipeline redis set_state writes, cache required-state classes#6745
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Sns7EuBPYvfGCxRSRZ1bUx
Greptile SummaryThis PR improves performance across state persistence, memo compilation, routing, and hot-path validation. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "Appease pyright in redis manager tests" | Re-trigger Greptile |
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9cd59d2f22
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Reset the touched flag so an unchanged state is not re-pickled | ||
| # and re-written by subsequent flushes of the same instance. | ||
| for substate in written_states: | ||
| substate._was_touched = False |
There was a problem hiding this comment.
the _was_touched flag was already being reset by virtue of BaseState.__setstate__ popping the flag out of the payload that gets saved in the pickle, so this is just added overhead now.
set_state recursed over the substate tree spawning a task per substate; every recursive call re-GETed the same lock key and re-PTTLed it, and each touched state got its own unpipelined SET -- roughly 2N+1 round trips per event flush for N states. The tree is now walked once, the lock is verified once, and all touched states are written in a single pipeline: 3 round trips per flush regardless of tree size (12->3 for a root with 3 substates, 39->3 for a 13-state tree). Also: - Pickling runs off the event loop (asyncio.to_thread) for states whose previous payload exceeded 64KiB, instead of stalling the loop. - The recursive required-state-classes computation (pure class-level data) is now lru-cached per registration context and state hierarchy generation; subclass creation and module hot reload bump the generation, so dynamic classes invalidate cleanly. - _was_touched is reset after a successful write (mirroring the disk manager), so a once-touched state held by the oplock cache is no longer re-pickled and re-SET on every subsequent flush. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Sns7EuBPYvfGCxRSRZ1bUx
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Sns7EuBPYvfGCxRSRZ1bUx
c4b7b46 to
4c6cc4f
Compare
|
Root cause of the The unit-tests job exports So Generated by Claude Code |
All Submissions:
Type of change
Changes To Core Features:
StateManagerRedis.set_staterecursed over the substate tree spawning a task per substate; every recursive call re-GETed the same lock key and re-PTTLed it, and each touched state got its own unpipelined SET — roughly 2N+1 redis round trips per event flush for N states.get_statealready pipelines its reads;set_statenow does the same for writes (ENG-10097):asyncio.to_threadinstead of stalling the event loop (safe: the token's lock is held, so nothing mutates the tree mid-pickle — the old code awaited between pickles too)._get_required_state_classes: the recursive class-level computation ran on everyget_state. It's now anlru_cached module function keyed on(target class, registration context, state hierarchy generation);BaseState.__init_subclass__andreload_state_modulebump the generation, so dynamically created classes and hot reload invalidate cleanly (including same-name class redefinition, where registry sizes don't change)._was_touchedreset after write (mirrors the disk manager's existingget_and_reset_touched_state): previously the flag was never cleared, so a once-touched state kept alive by the oplock cache was re-pickled and re-SET on every subsequent flush. Reset only happens after a successful pipeline execute, so failed writes retry.Round trips per event flush (modeled on the exact before/after command flows; "3" = lock GET + PTTL + one pipelined write):
New unit tests (run against both mock and real redis in CI):
test_set_state_verifies_lock_once_and_pipelines_writescounts actualGET/PTTLcommands during a 3-substate modify (asserts exactly one of each);test_set_state_resets_touched_flagasserts a second flush of an unchanged instance creates no write pipeline at all;test_set_state_offloads_large_picklesasserts theto_threadpath engages above the threshold;test_required_state_classes_cache_invalidationasserts a subclass defined after cache warm-up appears in the cached result via the generation bump. Existingmodify_state/oplock/expiration tests cover end-to-end behavior.Linear: ENG-10097
🤖 Generated with Claude Code
https://claude.ai/code/session_01Sns7EuBPYvfGCxRSRZ1bUx
Generated by Claude Code